Repository navigation
Conversation
Kimblebee
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for tracking and applying host OS display/window rotation (windowRotation) to the camera sessions, ensuring that preview framing, viewports, and stream-sharing buffers match the active window. It adds setWindowRotation across CameraSystem, CameraController, and their implementations, and updates CameraSessionEffect to handle window rotation updates, including freezing them during recording. Additionally, unit tests were added to verify these behaviors. The review feedback suggests simplifying the calls to createUseCaseGroup in both CameraSession and ConcurrentCameraSession by omitting the redundant windowRotation argument, as it already defaults to the initial transient settings' window rotation.
| captureResults = captureResults, | ||
| windowRotation = currentTransientSettings.windowRotation |
There was a problem hiding this comment.
The windowRotation argument is redundant here because createUseCaseGroup already defaults windowRotation to initialTransientSettings.windowRotation (which is currentTransientSettings.windowRotation in this call). We can omit it to simplify the call.
captureResults = captureResults| previewMirrorMode = sessionSettings.mirrorMode | ||
| .toCXConcurrentPreviewMirrorMode() | ||
| .toCXConcurrentPreviewMirrorMode(), | ||
| windowRotation = initialTransientSettings.windowRotation |
There was a problem hiding this comment.
The windowRotation argument is redundant here because createUseCaseGroup already defaults windowRotation to initialTransientSettings.windowRotation. We can omit it to simplify the call.
| previewMirrorMode = sessionSettings.mirrorMode | |
| .toCXConcurrentPreviewMirrorMode() | |
| .toCXConcurrentPreviewMirrorMode(), | |
| windowRotation = initialTransientSettings.windowRotation | |
| previewMirrorMode = sessionSettings.mirrorMode.toCXConcurrentPreviewMirrorMode() |
There was a problem hiding this comment.
Code Review
This pull request introduces support for tracking and applying host OS display/window rotation independently of physical device rotation. It adds "windowRotation" to camera sessions, transient settings, and camera controllers, allowing the preview target rotation to track window rotation dynamically. Additionally, "CameraSessionEffect" is updated to freeze window rotation updates during recording, and comprehensive tests are added to verify these behaviors. The feedback suggests a minor improvement in "CameraSession.kt" to use "filterIsInstance()" for a more idiomatic Kotlin implementation when updating the preview target rotation.
| useCaseGroup.useCases.forEach { useCase -> | ||
| if (useCase is Preview) { | ||
| useCase.targetRotation = targetRotation | ||
| } | ||
| } |
There was a problem hiding this comment.
Using filterIsInstance<Preview>() is a more idiomatic and concise way to filter and iterate over specific use case types in Kotlin, aligning with the pattern already used elsewhere in this file (e.g., line 1047).
useCaseGroup.useCases.filterIsInstance<Preview>().forEach { preview ->
preview.targetRotation = targetRotation
}References
- Simplify Complex Logic: Look for needlessly complex code. If a multi-line block of logic can be condensed into a more concise and readable idiomatic expression (e.g., using Kotlin standard library functions), suggest the simplification. (link)
Summary
Plumbs the host OS
windowRotationthroughTransientSessionSettingssoPreview.targetRotationupdates in place without rebinding the camera session whenever the window rotates (frozen while recording). Recent CameraX fixes (aosp/4408335,aosp/4408336,aosp/4408337) dynamically updateViewPortandStreamSharingcrop/rotation whenPreview.targetRotationchanges on a bound use case, allowingwindowRotationto live alongsidedeviceRotationinTransientSessionSettings. Sensor-alignedViewPortaspect ratio andCameraState.isPreviewLandscapeare coordinated with PR#592(TODO(#592)).Key Changes
TransientSessionSettings.windowRotation(CameraSessionSettings.kt,CameraSystem.kt,CameraXCameraSystem.kt,CameraSession.kt,ConcurrentCameraSession.kt,FakeCameraSystem.kt):windowRotation: DeviceRotation = DeviceRotation.NaturaltoTransientSessionSettingsandsetWindowRotationtoCameraSystem, updatingtransientSettingsin place inCameraXCameraSystemjust likesetDisplayRotation.Preview.Builder().setTargetRotation(windowRotation.toUiSurfaceRotation())and callapplyWindowRotation(newTransientSettings.windowRotation, useCaseGroup)inprocessTransientSettingEvents(and concurrent camera's transient settings collector) whenwindowRotationchanges, citingaosp/4408335,aosp/4408336,aosp/4408337and notingTODO(#592)onViewPortconstruction.CameraController.kt,CameraControllerImpl.kt,FakeCameraController.kt,PreviewScreen.kt,CameraSessionEffect.kt):windowRotation = LocalWindowRotation.currentfromPreviewScreenthroughCameraSessionEffect->CameraController.setWindowRotation.windowRotationupdates inpendingWindowRotationwhileisRecording == trueso active recordings never shift preview orientation mid-clip, flushing any queued rotation once recording stops.Technical Notes
TransientSessionSettings.windowRotationvsTransientSessionSettings.deviceRotation: Both signals live inTransientSessionSettingsand update bound use cases in place without rebinding:deviceRotationupdatesimageCapture.targetRotationandvideoCapture.targetRotation(for output file orientation metadata), whilewindowRotationupdatespreview.targetRotation(for live viewfinder buffer orientation, withaosp/4408335–4408337dynamically updatingViewPortandStreamSharing).#592: PR#592introducesgetViewPortAspectRatioandCameraState.isPreviewLandscape. KeepingwindowRotationfocused onPreview.targetRotationand markingViewPortconstruction withTODO(#592)avoids duplicateViewPorthelpers and ensures a clean merge when#592lands.Pre-Existing Bugs Fixed
CameraSession.kt,ConcurrentCameraSession.kt):Preview.targetRotationpreviously never updated when the host window rotated in multi-window or large-screen postures. UpdatingPreview.targetRotationfromTransientSessionSettings.windowRotationfixes this in place without tearing down the session.Not Included Yet
180°retention (kim/adaptive/content-gravity): Upright layout gravity for rectangular overlays (LandscapeTopEdgeBanner, Quick Settings landscape cross-fade and380.dpscroll cap,CameraErrorDialog,ZoomBarlabels,ToggleSwitchicons) and stateful sticky180°gravity retention land in the next branch.kim/adaptive/layout-orchestrator): Side-rail postures retain placeholders for verticalTopBarindicators (#649), verticalCaptureModeToggle(#635), andQuickSettingsOverlayCardPlaceholder(#648), plus independent leaf polish (#622,#644).ViewPortaspect ratio andisPreviewLandscape(#592): Handled in PR#592, which pairs withPreview.targetRotationonce both are merged intomain.W < HwithwindowRotation = 90°/270°): Once PR#592lands (CameraState.isPreviewLandscape),PreviewDisplaycan drive its aspect orientation from the oriented stream dimensions, falling back towindowRotation, so the uncropped frame letterboxes cleanly inside narrow split-screen panes.Testing
./gradlew :core:camera:testDebugUnitTest --tests "*CameraSessionTest*"— InitialPreview.targetRotationand in-placeTransientSessionSettings.windowRotationupdates across all four rotations without rebinding, whileImageCapture.targetRotationcontinues to trackdeviceRotationindependently +FakeCameraSystemtracking../gradlew :ui:controller:testing:testDebugUnitTest --tests "*FakeCameraControllerTest*"—setWindowRotationcallback verification../gradlew :ui:components:capture:testDebugUnitTest --tests "*CaptureScaffoldTest*"—CameraSessionEffectrecording freeze and flush for bothwindowRotationanddeviceRotation.